-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Make math primitives runtime constructable, meshable #20250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
1 similar comment
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this! I definitely prefer just using Vec
s for the general polygon and polyline types :)
Left some thoughts on a few methods and the ConvexPolygon
type
pub struct ConvexPolygon { | ||
/// The vertices of the [`ConvexPolygon`]. | ||
#[cfg_attr(feature = "serialize", serde(with = "super::serde::array"))] | ||
vertices: [Vec2; N], | ||
vertices: Vec<Vec2>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I'm less sure about, since convex polygons tend to have a small number of vertices, and you can have a lot of them (ex: as a result of convex decomposition). I imagine that ConvexPolygon
would primarily be used for collision detection or other geometry stuff where this can be relevant.
I wonder if the ideal representation would be to have a MAX_VERTICES
const generic and to store an ArrayVec
, that way you can store the vertices inline and have a cap based on how many vertices you expect to have 🤔 For example, for colliders I'd expect a max of 8 vertices per polygon. Box2D does basically the same (source).
Aside: Depending on what we intend ConvexPolygon
to be used for, we might also want to store the outward edge normals here. It's required for collision detection, computing support faces, and some other stuff. For example Parry and Box2D store them. That's out of scope for this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is to be able to make these runtime constructible, which if they use non-defaultable const-generics becomes a problem. For my use case, I'd prefer to have a bit more flexibility but I'm happy to subordinate that if there are real performance concerns. Just adding a const default with a Vec
constructor would be sufficient for me as it would allow constructing fewer than the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should definitely be a Vec. if a physics engine needs a faster primitive it should implement it itself, bevy_math should be first and foremost Usable and Ergonomic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want a faster version with fixed vertices we can add them again as Ngon<N: usize>
.
impl BoxedPolyline2d { | ||
/// Create a new `BoxedPolyline2d` from its vertices | ||
impl Polyline2d { | ||
/// Create a new `Polyline2d` from its vertices | ||
pub fn new(vertices: impl IntoIterator<Item = Vec2>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these just take a Vec
, considering from_iter
already exists if you want to use an iterator-like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would "never" (say never) use Vec as an argument if I can have impl. Maybe remove from_iter
instead?
If we change it, please add a comment to refer to from_iter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_iter
here comes from the FromIterator
trait, which makes sense to have implemented IMO.
I'm not sure if Rust does some special optimization here, but mainly my motivation is that from_iter
does iter.into_iter().collect()
, which (I believe) allocates a new vector, unless Rust is smart enough to figure out when it can just use the original vec. For most of my use cases I would instead want to move the given vector directly without doing any unnecessary allocations.
But you can also just not use the constructor at all and do Polyline2d { vertices: todo!() }
, so it's not a big problem regardless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should re-use the allocation in almost all instances but would have to double check under real use.
Briefly looking through the issue tracker rust-lang/rust#110353 suggests that this optimization is enabled for situations beyond the likely simple case here of passing a vec in directly.
@tychedelia, I'm waiting on your response to feedback on this PR currently :) Once that's done, ping me for a review please. |
Cutting from the milestone. This isn't critical or ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! The convex polygon thing is something we can revisit later if it comes up, I'm fine with the Vec
storage for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive been loosely following this, i largely agree with this direction
I agree with the tradeoff and direction made here, and we can readily add fixed-size ngons back if we find a strong use case. |
Objective
Many math primitives require const generics and thus are not constructable at runtime and also not meshable. While the use of const generics is theoretically more performant, it makes them very difficult to interact with in a generic way, particularly in relationship to mesh construction. For example, a ui that would allow selecting a primitive in order to create a mesh.
Solution
Make them alloc and meshable.